Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix aggregate functions to return numeric value consistently even on custom attribute type #39039

Merged
merged 1 commit into from
Apr 25, 2020

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Apr 24, 2020

Currently, count and average always returns numeric value, but
sum, maximum, and minimum not always return numeric value if
aggregated on custom attribute type.

I think that inconsistent behavior is surprising:

# All adapters except postgresql adapter are affected
# by custom type casting.

Book.group(:status).sum(:status)
# => { "proposed" => "proposed", "published" => nil }

That is caused by fallback looking up cast type to type_for(column).
Now all supported adapters can return numeric value without that
fallback, so I think we can remove that, it will also fix aggregate
functions to return numeric value consistently.

type = result.column_types.fetch(column_alias) do
type_for(column_name)
end
type = result.column_types.fetch(column_alias, Type.default_value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it strange that column_alias is nil here for count calculations with a subquery.

All in-tree adapters return integers from count queries, so how about dropping the type casting from that branch entirely?

diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index f6d6821f72..658c9c84e8 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -291,6 +291,7 @@ def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
           return 0 if limit_value == 0

           query_builder = build_count_subquery(spawn, column_name, distinct)
+          skip_query_cache_if_necessary { @klass.connection.select_value(query_builder) }
         else
           # PostgreSQL doesn't like ORDER BY when there are no GROUP BY
           relation = unscope(:order).distinct!(false)
@@ -307,14 +308,14 @@ def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
           relation.select_values = [select_value]

           query_builder = relation.arel
+
+          result = skip_query_cache_if_necessary { @klass.connection.select_all(query_builder) }
+          row    = result.first
+          value  = row && row.values.first
+          type   = result.column_types.fetch(column_alias, Type.default_value)
+
+          type_cast_calculated_value(value, type, operation)
         end
-
-        result = skip_query_cache_if_necessary { @klass.connection.select_all(query_builder, nil) }
-        row    = result.first
-        value  = row && row.values.first
-        type   = result.column_types.fetch(column_alias, Type.default_value)
-
-        type_cast_calculated_value(value, type, operation)
       end

       def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Updated.

…custom attribute type

Currently, `count` and `average` always returns numeric value, but
`sum`, `maximum`, and `minimum` not always return numeric value if
aggregated on custom attribute type.

I think that inconsistent behavior is surprising:

```ruby
# All adapters except postgresql adapter are affected
# by custom type casting.

Book.group(:status).sum(:status)
# => { "proposed" => "proposed", "published" => nil }
```

That is caused by fallback looking up cast type to `type_for(column)`.
Now all supported adapters can return numeric value without that
fallback, so I think we can remove that, it will also fix aggregate
functions to return numeric value consistently.
@kamipo kamipo force-pushed the fix_aggregate_on_custom_type branch from afee04d to 89e1dd6 Compare April 25, 2020 05:17
@kamipo kamipo merged commit e63ab64 into rails:master Apr 25, 2020
@kamipo kamipo deleted the fix_aggregate_on_custom_type branch April 25, 2020 18:47
kamipo added a commit to kamipo/rails that referenced this pull request Apr 29, 2020
Related rails#39039.

Currently PostgreSQL adapter is the only adapter that rely on type
casting by `result.column_types`, now the adapter can return numeric
value without type casting by Active Record side.
So we can remove that useless type cast on aggregated value.
kamipo added a commit to kamipo/rails that referenced this pull request May 2, 2020
I supposed all aggregation functions will return numeric result in
rails#39039, but that assumption was incorrect for `minimum` and `maximum`,
if an aggregated column is non numeric type.

I've restored type casting aggregated result for `minimum` and `maximum`.

Fixes rails#39110.
kamipo added a commit to kamipo/rails that referenced this pull request May 12, 2020
This is the opposite direction of rails#39039.

rails#39111 fixes `minimum` and `maximum` on date columns with type casting
by column type on the database. But column type has no information for
time zone aware attributes, it means that attribute type should always
be precedence over column type. I've realized that fact in the related
issue report rails#39248.

I've reverted the expectation of rails#39039, to make time zone aware
attributes works.
kamipo added a commit to kamipo/rails that referenced this pull request May 13, 2020
…e type

Follow up to rails#39255, rails#39039.

One of the purpose of this was to unify the behavior between the
databases.

Original code was:

```ruby
    type   = result.column_types.fetch(column_alias) do
      type_for(column_name)
    end
```

The code will attempt looking up type from `column_types`, then fallback
to attribute types, so I supposed the code was originally intended to
cast a value by the database types.

But now, most modern clients will already return casted values, and no
longer use `column_types`, except Postgres.
As a result, now most adapter accidentally fallback to attribute types.
Since casted by attribute types sometimes doesn't return numeric values,
I've unified the behavior to use database types consistently in rails#39039.

But later, I've learned that attribute types have important settings
like time zone aware attributes (rails#39255), and some existing code relying
on attribute types over database types (rails#39271).

I've changed all aggregated values are casted by attribute types.

Fixes rails#39271.
kamipo added a commit to kamipo/rails that referenced this pull request Mar 4, 2021
To address the issues rails#39248 and rails#39271, rails#39255 and rails#39274 made
aggregated results type casted by the attribute types.

But I've realised that we could avoid enum mapping when implemented
rails#41431.

This change restores the expectation of rails#39039 especially in the Enum
case.

Fixes rails#41600.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants